Skip to content

Add support for Raspberry Pi Pico 2 flash controller. #89182

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add support for Raspberry Pi Pico 2 flash controller. #89182

wants to merge 3 commits into from
+17 −168

Conversation

hanan619
Copy link

The original Pico uses an SSI flash controller, which already has support
in Zephyr. The Pico 2 uses a different QMI flash controller. This PR adds
QMI controller support to Zephyr:

  • Added QMI controller implementation in flash_rpi_pico.c
  • Enabled the flash controller node in rp2350.dtsi

@github-actions github-actions bot added area: Flash platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) labels Apr 28, 2025
@hanan619 hanan619 mentioned this pull request Apr 28, 2025
Open
@ajf58
Copy link
Collaborator

ajf58 commented Apr 28, 2025

@hanan619 it looks like you've copied code directly from the Pico SDK. This is not permitted: the Pico SDK is under the BSD 3-Clause, and Zephyr is Apache 2.0

ajf58
ajf58 requested changes Apr 28, 2025
View reviewed changes
Copy link
Collaborator

@ajf58 ajf58 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have some clarification about the provenance of the changes before reviewing further.

@hanan619
Copy link
Author

I'd like to have some clarification about the provenance of the changes before reviewing further.

I’d like to point out that the existing RP2040 (Pico1) flash implementation in Zephyr also follows the Pico SDK and includes direct references to its source. My implementation for RP2350 (Pico2) continues this approach for consistency across Raspberry Pi platforms.
Regarding licensing: the Pico SDK is under the BSD-3-Clause license, which is permissive and compatible with Apache 2.0, provided proper attribution is given. Since Zephyr already contains BSD-licensed contributions (such as the RP2040 code), I believe this implementation does not introduce a new licensing concern.
To make this even clearer, I updated the attribution comment in the code to explicitly cite the relevant sources used for RP2350, including:
• pico-bootrom for generic flash logic
• pico-bootrom-rp2350 for RP2350-specific logic
• and the pico-sdk flash code
Given that the RP2040 driver follows this same pattern and was accepted, I believe the same should apply to RP2350.
Please let me know if you would prefer the attribution comment be adjusted further to meet Zephyr’s contribution guidelines more explicitly.

@hanan619 hanan619 force-pushed the rpi_pico2_flash_support branch 8 times, most recently from ad66107 to 8cc9df8 Compare April 29, 2025 06:10
@SrikanthGoud123 SrikanthGoud123 mentioned this pull request Apr 29, 2025
Merged
@ajf58
Copy link
Collaborator

ajf58 commented Apr 29, 2025

I’d like to point out that the existing RP2040 (Pico1) flash implementation in Zephyr also follows the Pico SDK and includes direct references to its source.

This https://github.com/zephyrproject-rtos/zephyr/pull/89182/files#diff-5b63ca534e31287da5b9512b307ebf0a52703e12edb4bece40956c0c2bef0b48R2-R3 certainly is a little concerning: Raspberry Pi haven't offered up anything under Apache 2.0 . Re-labelling Raspberry Pi's BSD 3-Clause as Apache 2.0 is not permitted. I'll raise this concern in an issue, and edit this message with a link to it here.

My implementation for RP2350 (Pico2) continues this approach for consistency across Raspberry Pi platforms.

I don't ascribe to a mindset of "everyone else is misbehaving, so I'll misbehave too." I wasn't a reviewer of the previous code, so my focus is brought to this here.

Regarding licensing: the Pico SDK is under the BSD-3-Clause license, which is permissive and compatible with Apache 2.0, provided proper attribution is given.

  1. Proper attribution hasn't been given: clause 1 of the BSD 3 Clause (emphasis mine):

Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.

That list of conditions (and/or the reference to the code being released under BSD 3 Clause.

  1. The Zephyr project has its own rules/policies regarding mixing in code released under different licences.

Please let me know if you would prefer the attribution comment be adjusted further to meet Zephyr’s contribution guidelines more explicitly.

Thanks, I think it's best for some of the more experienced reviewers to comment on this. @henrikbrixandersen , I think you've contirbuted feedback on similar licencing terms, would you be able to comment?

kartben
kartben previously requested changes Apr 30, 2025
View reviewed changes
Copy link
Collaborator

@kartben kartben left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From https://docs.zephyrproject.org/latest/contribute/guidelines.html#licensing:
Importing code into the Zephyr OS from other projects that use a license other than the Apache 2.0 license needs to be fully understood in context and approved by the Zephyr governing board.

Can't your contribution just be (your) Zephyr glue code around code being pulled from the pico-sdk zephyr module?

@hanan619
Copy link
Author

hanan619 commented May 6, 2025 •
edited
Loading

Partial writes are not supported in pico-sdk. Therefore someone has added partial_write implementation in zephyr flash for rp2040.
I think we can remove the partial write support for both rp2040 and rp2350 and let the pico-sdk team add proper support in bootrom repo.
There is already an issue raised and work in progress for this task
raspberrypi/pico-sdk#1169

@soburi
Copy link
Member

soburi commented May 14, 2025

@hanan619

It seems that the check was missed in the original code, so we need to consider how to deal with it.

Here, the request is to move the HAL-derived sources to https://github.com/zephyrproject-rtos/hal_rpi_pico rather than https://github.com/raspberrypi/pico-sdk

so there is no problem with putting the zephyr-specific modification, and no required upstreaming.

I think it is possible to move the HAL-derived sources and their modifications from here to

https://github.com/zephyrproject-rtos/hal_rpi_pico/tree/zephyr/src/rp2_common/hardware_flash.

@hanan619 hanan619 force-pushed the rpi_pico2_flash_support branch from 8cc9df8 to 840defa Compare May 19, 2025 09:38
@hanan619
Copy link
Author

the zephyr-specific modification, and n

I have created a PR in hal_rpi_pico for the desired changes.
zephyrproject-rtos/hal_rpi_pico#11

@hanan619 hanan619 force-pushed the rpi_pico2_flash_support branch from 840defa to 58c17d3 Compare May 19, 2025 12:05
@github-actions GitHub Actions
Copy link

github-actions bot commented May 19, 2025 •
edited
Loading

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff
hal_rpi_pico zephyrproject-rtos/hal_rpi_pico@7b57b24 zephyrproject-rtos/hal_rpi_pico@5a981c7 (zephyr) zephyrproject-rtos/hal_rpi_pico@7b57b245..5a981c7c

✅ All manifest checks OK

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@kartben kartben dismissed their stale review May 22, 2025 13:34

removing my block since licensing issues seem to be under control

@hanan619 hanan619 force-pushed the rpi_pico2_flash_support branch from a94ad47 to 5369096 Compare May 22, 2025 14:04
@hanan619
Copy link
Author

Can anyone help me with the failed tests?

@kartben
Copy link
Collaborator

kartben commented May 22, 2025

Can anyone help me with the failed tests?

please check you're using the correct format specifiers based on the variable types.

@hanan619
Copy link
Author

hanan619 commented May 22, 2025 •
edited
Loading

please check you're using the correct format specifiers based on the variable types.

In this PR, there is no change which involves format specifiers

@kartben kartben closed this May 22, 2025
@kartben kartben reopened this May 22, 2025
@kartben
Copy link
Collaborator

kartben commented May 22, 2025

re-triggering CI -- a change was just merged in main that should make it green again 🤞🏼

@hanan619
Copy link
Author

@soburi did you got time to review the PR zephyrproject-rtos/hal_rpi_pico#11

@soburi
Copy link
Member

soburi commented May 27, 2025

@soburi did you got time to review the PR zephyrproject-rtos/hal_rpi_pico#11

Sorry for the wait.
I'm starting to look at it. I should be able to comment within the day.

@soburi soburi mentioned this pull request May 30, 2025
Open
55 tasks
@hanan619
Copy link
Author

hanan619 commented Jun 4, 2025

@munir-zin reference

@soburi
Copy link
Member

soburi commented Jun 9, 2025

@kartben

#89182 (review)

The license issue addressed aligned with the hal repository.
Could you review also it if you have a time, please?

zephyrproject-rtos/hal_rpi_pico#11

@soburi
Copy link
Member

soburi commented Jun 15, 2025

zephyrproject-rtos/hal_rpi_pico#11 merged.
Please update and repush this.

@hanan619 hanan619 force-pushed the rpi_pico2_flash_support branch 2 times, most recently from 18da061 to ad6ce72 Compare June 16, 2025 06:49
hanan619 added 2 commits June 16, 2025 11:50
@hanan619
drivers: flash: rpi_pico: add support for rp2350 flash controller
d8e8fa3 
The Raspberry Pi Pico 2 uses a QMI flash controller, which differs from the
SSI controller used in the original Pico. Zephyr already has support for
the SSI controller, but lacked support for QMI.

This change adds the QMI flash controller implementation in the
flash_rpi_pico.c driver file. Additionally, the RP2350 SoC devicetree file
(rp2350.dtsi) has been updated to enable and describe the flash controller
for Pico 2.

Signed-off-by: Hanan Arshad <hananarshad619@gmail.com>
@hanan619
drivers: flash: flash_rpi_pico: fix indentation and remove unused var…
bae535e 
…iables

Cleaned up the flash_rpi_pico driver to improve code readability and
compliance with Zephyr coding standards. Fixed inconsistent indentation
across the file and removed variables that were declared but not used.

A few improvements are credited to Manu3l0us.

Signed-off-by: Hanan Arshad <hananarshad619@gmail.com>
@hanan619 hanan619 force-pushed the rpi_pico2_flash_support branch from ad6ce72 to b716e35 Compare June 16, 2025 06:51
@hanan619
manifest: hal_rpi_pico: Pull partial flash write changes from hal_rpi…
63fc23a 
…_pico

This commit links the hal_rpi_pico PR required for the flash driver.
west.yml is updated

Signed-off-by: Hanan Arshad <hananarshad619@gmail.com>
@hanan619 hanan619 force-pushed the rpi_pico2_flash_support branch from b716e35 to 63fc23a Compare June 16, 2025 06:57
@github-actions github-actions bot removed the DNM (manifest) This PR should not be merged (controlled by action-manifest) label Jun 16, 2025
@hanan619
Copy link
Author

@soburi PR updated

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@hanan619
Copy link
Author

@soburi Please review

@soburi
Copy link
Member

soburi commented Jun 19, 2025

@kartben

zephyrproject-rtos/hal_rpi_pico#11 (comment)

I would like to ask you to confirm from license perspective again.

@soburi
Copy link
Member

soburi commented Jun 26, 2025

@ajf58

Could you take a look if you have a time, please?

@dkalowsk dkalowsk added this to the v4.3.0 milestone Jun 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soburi soburi soburi approved these changes

@ajf58 ajf58 ajf58 approved these changes

@de-nordic de-nordic Awaiting requested review from de-nordic

@rghaddab rghaddab Awaiting requested review from rghaddab

@ThreeEights ThreeEights Awaiting requested review from ThreeEights

@yonsch yonsch Awaiting requested review from yonsch

@kartben kartben Awaiting requested review from kartben

Assignees

@soburi soburi

Labels
area: Flash manifest manifest-hal_rpi_pico platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico)
Projects
None yet
Milestone
v4.3.0
Development

Successfully merging this pull request may close these issues.
5 participants
@hanan619 @ajf58 @soburi @kartben @dkalowsk